Repository navigation
Conversation
|
|
||
| ```cpp | ||
| Status ReadOffsets(const std::string& path, int64_t limit, MemoryPool* pool, | ||
| std::vector<int64_t>* offsets); |
There was a problem hiding this comment.
Is this a positive example? Can we avoid using pool? Since both pool and offsets are out parameters, it’s hard to determine their ordering.
| - Give default-constructible structs and classes an explicit initial state. Initialize scalar | ||
| members in their declarations or in every constructor that can create the object. | ||
| - A default value must represent a meaningful state. Require business-critical inputs through a | ||
| constructor or `Create()` rather than supplying a placeholder solely to allow default construction. |
There was a problem hiding this comment.
If some parameters are passed in during Create(), how can we provide a meaningful default value for them?
|
|
||
| For example, extracting schema validation from a function that also opens files can clarify both | ||
| operations. Wrapping a single expression used once may only add another place for readers to look. | ||
|
|
There was a problem hiding this comment.
Before merging the code, please research whether other open-source libraries have such detailed guidelines.
|
|
||
| This contributing guide is the entry point for setup, validation, contribution workflow, and | ||
| external code attribution. Link to the detailed rules rather than maintaining parallel copies. | ||
|
|
There was a problem hiding this comment.
“Link to the detailed rules rather than maintaining parallel copies.” What does this mean?”
| When both layers need a contract, put the shared interface in an appropriate common layer and | ||
| let the plugin implement it. | ||
| - Keep a feature's readers, writers, and related utilities together in the module that owns the | ||
| behavior. Put a helper in a shared utility directory only if it has a suitable shared purpose. |
There was a problem hiding this comment.
I’m not sure whether this might conflict with the compatibility Java logic. Many helper functions are currently placed under common/utils even though they’re only used in a specific module—because we align files with Java. For this case, please keep it aligned with Java as well, to improve consistency and verification.
|
|
||
| | Outcome | Contract | | ||
| |---------|----------| | ||
| | Operation succeeded with no data | A valid empty result, or a documented end-of-input representation | |
|
|
||
| - Exercise the affected production entry point and assert behavior that would expose the target | ||
| defect. When practical, confirm that the test fails with the fix removed. A test that reproduces | ||
| an algorithm locally without calling the changed production code does not cover that code. |
There was a problem hiding this comment.
Can you explain “A test that reproduces
an algorithm locally without calling the changed production code does not cover that code.”?
| details and fail after a refactor that preserves the contract. | ||
|
|
||
| For example, a fix to read-ahead behavior needs a test that reads through the affected reader or | ||
| cache entry point. A separate byte-copy exercise cannot establish that the reader uses the fix. |
There was a problem hiding this comment.
This seems to be an explanation of the previous point, but I’m not quite sure I understand.
Purpose
Linked issue: N/A (documentation-only change).
Coding conventions and contribution workflow are currently mixed in the code style guide, while several design and testing expectations are implicit. This change gives each topic a primary home:
Tests
No tests are added or modified because this change only updates contributor documentation and does not change runtime behavior.
API and Format
No changes to public APIs, storage formats, or protocols.
Documentation
Updates CONTRIBUTING.md, docs/code-style.md, and AGENTS.md; adds docs/development-guidelines.md.
Generative AI tooling
Generated-by: Codex CLI 0.163.0